fix(macos): prevent stop_mac_app from terminating unrelated processes#484
Conversation
Stop macOS apps by exact executable name instead of matching the app name against every process argument. Reject empty app names and unsafe process IDs before command execution. Fixes #306
commit: |
There was a problem hiding this comment.
Pull request overview
This PR hardens the stop_mac_app tool’s process targeting on macOS by switching from command-line substring matching to exact process-name matching, preventing accidental termination of unrelated processes (e.g., tmux, shells) whose arguments contain the app name.
Changes:
- Tightened tool input validation:
appNamemust be non-empty andprocessIdmust be a positive integer. - Changed name-based termination from
pkill -ftopkill -x(exact process name), with escaping to prevent regex interpretation. - Updated unit tests and added a changelog entry for the behavior change and validation hardening.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mcp/tools/macos/stop_mac_app.ts | Updates schema validation and switches pkill invocation to exact-name matching with regex escaping. |
| src/mcp/tools/macos/tests/stop_mac_app.test.ts | Expands schema validation assertions and updates command-generation expectations for pkill -x. |
| CHANGELOG.md | Documents the fix under ## [Unreleased] and links it to issue #306. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reject unsafe process IDs before constructing or invoking kill, even when callers bypass the typed-tool schema. Refs #306
|
CI status note: all repository-controlled checks pass and all review threads are resolved on the current head. The remaining external GitHub marks the external suite rerequestable, but the documented check-suite rerequest endpoint returns 404 for the maintainer token, so only the Warden app/service owner can clear these app-owned statuses. No repository code failure remains to address. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
cameroncooke
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6f59c5a. Configure here.
Use killall instead of full-command regular expressions so later arguments cannot select unrelated processes. Refs #306
Name the unit test for its command-construction boundary; host-level validation covers actual process selection. Refs #306
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Problem
stop_mac_appstops a macOS application after XcodeBuildMCP launches it. A caller can target one process by ID, or target running processes by the application's executable name.Name-based stops previously searched every process's full command line for the supplied name. Stopping an app such as
Brimdaycould therefore also terminate tmux, a shell, an editor, or an agent process that merely containedBrimdayin its arguments. In the reported case this detached the developer's tmux session and interrupted the workflow instead of only stopping the app.The process-ID path also accepted unsafe values when internal callers invoked the execution logic without going through the public schema. PID
0is especially dangerous becausekillinterprets it as the caller's entire process group.Fix
Name-based stops now use macOS
killallwith the application executable name.killallselects processes by their process name rather than searching their command-line arguments, so a matching project path, tmux session, shell command, or other later argument cannot select an unrelated process. The--separator prevents an application name beginning with-from being interpreted as an option, and long executable names remain supported.The public schema and the execution boundary now both reject empty application names and process IDs that are non-positive, fractional, non-finite, or outside JavaScript's safe-integer range before invoking a system command. Invalid process IDs are removed from failure artifacts so those error responses still satisfy the published structured-output schema.
The external interface is unchanged. A process ID still targets one process and takes precedence when both inputs are present. An application name still stops every process whose executable has exactly that name.
Regression protection
Focused tests cover name-based command construction, long executable names, names containing option or regular-expression characters, process-ID precedence, and every rejected process-ID class. The invalid-ID tests also prove that no system command is executed and that the emitted failure artifacts contain no invalid process ID. The smoke harness verifies the exact command exposed through the MCP tool boundary.
The selection behavior was additionally verified against real macOS processes: a target with a long executable name was stopped successfully, while another process carrying
/tmp/<target-name>only as a later argument remained running. An executable name beginning with-was also stopped successfully through the option boundary.The full unit suite passes with 2,903 tests, together with structured-output schema tests, fixture schema validation, type checking, the production build, formatting, and linting. Snapshot and smoke suites were not run because repository policy requires separate approval.
Fixes #306.